🤖 Codex PR Review

🚫 Merge blocked: 3 serious issue(s) found in high-risk files

⚖️ Codex Review Arbitration

🚫 block: The current blockers remain valid against the cumulative diff. First, _finding_matches_trusted_advisory() only binds suppression to advisory_provenance + file + overlapping advisory line range; it never checks that the current finding is the same defect semantically, so a different high/critical bug on the same line can still be marked trusted_advisory_suppressed and stop blocking. That is the same required behavior as the prior blocker, not a contract conflict. Second, evaluate_findings() accepts new_reachability_evidence whenever _parse_blocking_evidence() can parse any different existing repo line; it does not verify that the new caller/boundary belongs to the same advisory or scope, and the added test test_genuinely_different_reachability_identity_is_new explicitly codifies re-blocking from an unrelated entrypoint. Third, _history_finding_identity() still omits line/advisory identity and remaining_history_after_trusted_advisory_suppression() removes active history by that coarse key, so same-file same-description unresolved blockers can be dropped together when only one advisory was actually suppressed.

🚫 Blocking Issues

These issues must be fixed before this PR can be merged:

1. 🟠 [HIGH] Logic in scripts/run_codex_pr_review.py

On unchanged heads, _finding_matches_trusted_advisory() treats a finding as the same resolved advisory when advisory_provenance, file, and line range match, but it never checks that the current finding is semantically the same issue. That means a new reachable defect on the same line can copy a valid authenticated provenance from the prompt, get marked trusted_advisory_suppressed, and main() will stop blocking on a real current bug. (line 1020)

Suggestion: Bind suppression to a bridge-generated stable advisory identity for the exact prior finding (for example thread id plus classification and prior fingerprint), not just file/line/provenance echoed back by the model.

2. 🟠 [HIGH] Logic in scripts/run_codex_pr_review.py

new_reachability_evidence is accepted whenever it names any other existing repository line with a valid kind|path|line|symbol shape. In the unchanged-head path, a repeated advisory can therefore become blocking again just by emitting an unrelated caller/boundary from elsewhere in the checkout, even though there is no materially new reachability for that advisory under the current PR. (line 1092)

Suggestion: Validate new_reachability_evidence against the same advisory identity and finding scope, or only honor new reachability evidence that is authenticated by the bridge instead of any syntactically valid repository line.

3. 🟠 [HIGH] Logic in scripts/run_codex_pr_review.py

_history_finding_identity() collapses findings by (severity, category, file, normalized description) and omits the line number and advisory identity. When one unchanged-head advisory is suppressed, remaining_history_after_trusted_advisory_suppression() can also remove a different unresolved blocker in the same file that happens to share the same description text, which skips arbitration and incorrectly clears active history. (line 1235)

Suggestion: Include the finding line and stable advisory identity/provenance in _history_finding_identity() so suppression only exempts the exact previously resolved blocker.

ℹ️ Other Findings

1. 🟡 [MEDIUM] Reliability in scripts/run_codex_pr_review.py

The GraphQL query only loads comments(last: 100) for each review thread. Any resolved thread with more than 100 comments can have its authenticated disposition comment paged out of this window, so exact-head trusted advisory metadata is silently dropped and unchanged-head repeats start blocking again or triggering contract-conflict handling even though a valid disposition still exists. (line 1708)

Suggestion: Paginate thread comments or fetch disposition comments separately instead of hard-capping each thread to the last 100 comments.

Review by Codex PR Review bot • PR